Skip to content

feat: add operation for cycling crosswalk tags#1160

Merged
bhousel merged 21 commits intofacebook:mainfrom
RitaDee:crosswalk
Nov 8, 2023
Merged

feat: add operation for cycling crosswalk tags#1160
bhousel merged 21 commits intofacebook:mainfrom
RitaDee:crosswalk

Conversation

@RitaDee
Copy link
Copy Markdown
Contributor

@RitaDee RitaDee commented Oct 20, 2023

Description

This PR introduces a new feature (an operation function) that enables users to easily cycle through crosswalk tags within the OSM editing context. The operation supports keyboard shortcuts, provides a tooltip with descriptive information, and includes annotation.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes: #1154

Mobile & Desktop Screenshots/Recordings

Before:

Screen.Recording.2023-11-01.at.09.26.14.mov

After:

Screen.Recording.2023-11-01.at.09.29.54.mov

Added tests?

Uploading Screenshot 2023-11-01 at 09.42.26.png…

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 🙅 no documentation needed

Copy link
Copy Markdown
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments about strings and the regex used to determine the feature's appropriateness.

Comment thread modules/operations/cycle_crosswalk_tag.js Outdated
Comment thread modules/operations/cycle_crosswalk_tag.js Outdated
Copy link
Copy Markdown
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @RitaDee - instead of creating an entirely new operation, let's instead augment the existing cycle highway tags operation to do both things. I apologize for not making this requirement clearer in my original issue writeup.

We'll need you to take the following actions:

  1. Move the existing cycle crosswalk tag functionality into the main cycle_highway_tag.js file.
  2. Ensure that it works in both cases: highways get their tags cycled correctly, AND crosswalks get their tags cycled correctly.
  3. Remove the new cycle_crosswalk_tag: core.yaml entry and subentries for title, key, description, but keep the new annotation and put it under the cycle_highway_tag entry.

I hope that makes sense!

@RitaDee
Copy link
Copy Markdown
Contributor Author

RitaDee commented Oct 23, 2023

@Bonkles, it does make sense.

@RitaDee RitaDee marked this pull request as ready for review October 23, 2023 23:15
Copy link
Copy Markdown
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs refactoring! The highway logic and crossing logic should be kept separate, depending on what the user has selected, we should conditionally do one thing (cycle a crossing's tags) or the other (cycle a highway's tags).

Comment thread data/core.yaml Outdated
Comment thread modules/operations/cycle_highway_tag.js Outdated
Comment thread modules/operations/cycle_highway_tag.js Outdated
@RitaDee RitaDee marked this pull request as draft October 25, 2023 07:18
Comment thread modules/operations/cycle_highway_tag.js Outdated
Comment thread modules/operations/cycle_highway_tag.js Outdated
Copy link
Copy Markdown
Contributor

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple comments directly on the cycle_highway_tags source!

@bhousel bhousel marked this pull request as ready for review November 8, 2023 17:07
@bhousel
Copy link
Copy Markdown
Contributor

bhousel commented Nov 8, 2023

This looks pretty good to me and the tests are passing, so I'm going to squash-merge it back to main.
I resolved the conflicts so it should now be cycling through the presets that already landed on main in #1197
(It's not good to have branches hang around for weeks - stuff moves fast here!)

@bhousel bhousel merged commit 30dce47 into facebook:main Nov 8, 2023
Bonkles added a commit that referenced this pull request Nov 8, 2023
Bonkles added a commit that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keyboard Shortcut idea: cycle through common footway types

4 participants